[ty] provide `import` completion when in `from <name> <name>` statement#21291

Merged

BurntSushi merged 1 commit intoastral-sh:mainfrom

MatthewMckee4:no-completions-in-from-import

Nov 10, 2025

Contributor

MatthewMckee4 commented Nov 6, 2025 •

edited

Loading

Summary

Resolves astral-sh/ty#1494

Test Plan

Add a test showing if we are in from <name> <name> we provide the keyword completion "import"

MatthewMckee4 requested review from AlexWaygood, MichaReiser, carljm, dcreager and sharkdp as code owners

November 6, 2025 12:09

MatthewMckee4 changed the title ~~[ty] Don't provide completions when in 'from <name> i' statement~~ [ty] Don't provide completions when in 'from <name> <name>' statement

MatthewMckee4 changed the title ~~[ty] Don't provide completions when in 'from <name> <name>' statement~~ [ty] Don't provide completions when in from <name> <name> statement

Contributor

github-actions bot commented Nov 6, 2025 •

edited

Loading

Diagnostic diff on typing conformance tests

No changes detected when running ty on typing conformance tests ✅

Contributor

github-actions bot commented Nov 6, 2025 •

edited

Loading

`mypy_primer` results

No ecosystem changes detected ✅
No memory usage changes detected ✅

MatthewMckee4 marked this pull request as draft

November 6, 2025 12:19

MatthewMckee4 changed the title ~~[ty] Don't provide completions when in from <name> <name> statement~~ [ty] Don't import completion when in from <name> <name> statement

MatthewMckee4 marked this pull request as ready for review

November 6, 2025 12:37

MatthewMckee4 changed the title ~~[ty] Don't import completion when in from <name> <name> statement~~ [ty] provide import completion when in from <name> <name> statement

AlexWaygood requested a review from BurntSushi

November 6, 2025 12:57

AlexWaygood added server ty labels

MichaReiser removed request for AlexWaygood, carljm, dcreager and sharkdp

November 6, 2025 13:13

MichaReiser approved these changes

View reviewed changes

Member

MichaReiser left a comment

Only showing import in a from <NAME> <NAME2> position does make sense to me. I was surprised to see that Pylance shows all completions, but gives import a higher ranking

Contributor Author

MatthewMckee4 commented Nov 6, 2025

For some reason i dont get completions in playground locally for import when i have just a

This could be a playground thing though?

Member

MichaReiser commented Nov 6, 2025

I just checked out this PR and built the playground and this works as expected when you have from types i but it doesn't work when you have from types aa.

This could either be because the playground applies its own filtering (not sure if there's a way for us to disable it, do you want to take a look) or because our own "text prefix matching"

Member

BurntSushi commented Nov 6, 2025

Yeah we may not be able to fully control whether import is shown for from module import aa<CURSOR>. LSP clients can and will do their own filtering AIUI. I'm not sure if there is a foolproof way to override that.

AlexWaygood reviewed

View reviewed changes

Member

AlexWaygood left a comment

Nice, thank you!

crates/ty_ide/src/completion.rs Outdated Show resolved Hide resolved

crates/ty_ide/src/completion.rs Show resolved Hide resolved

Contributor Author

MatthewMckee4 commented Nov 6, 2025

Thank you, I will revisit this in a few days, when I am back home.

BurntSushi reviewed

View reviewed changes

crates/ty_ide/src/completion.rs Show resolved Hide resolved

AlexWaygood reviewed

View reviewed changes

crates/ty_ide/src/completion.rs Show resolved Hide resolved

AlexWaygood reviewed

View reviewed changes

crates/ty_ide/src/completion.rs Outdated Show resolved Hide resolved

AlexWaygood approved these changes

View reviewed changes

Member

AlexWaygood left a comment

Thank you!

AlexWaygood requested a review from BurntSushi

November 10, 2025 00:47

MichaReiser reviewed

View reviewed changes

crates/ty_ide/src/completion.rs Outdated

+                      return None;
+                  }
+                  let from_index = tokens.iter().position(|t| t.kind() == TokenKind::From)?;

Member

MichaReiser Nov 10, 2025

The search here seems to be too permissive to me. What's preventing it from going across logical lines?

Can you add some tests with multiple statements? I suspect that we now incorrectly match on import completions if there's any import statement before <CURSOR>.

Contributor Author

MatthewMckee4 Nov 10, 2025

Good point, will add that

MichaReiser reviewed

View reviewed changes

crates/ty_ide/src/completion.rs Outdated

+                      return None;
+                  }
+                  let from_index = tokens.iter().rposition(|t| t.kind() == TokenKind::From)?;

Member

MichaReiser Nov 10, 2025

I still think this is incorrect as it cross logical line boundaries. E.g. what if

from module impo

a<CURSOR>

We would want to show completions for a and not suggest import

Contributor Author

MatthewMckee4 Nov 10, 2025

I am maybe misunderstanding you. we do still provide completions for a here.

Member

MichaReiser Nov 10, 2025

I haven't checked out the PR. What I'm concerned with the current approach is that tokens is anything before <CURSOR> (unless this isn't the case?). That means, it's possible for tokens.iter().rposition() to find the from even though it belongs to an entirely different statement.

BurntSushi requested changes

View reviewed changes

crates/ty_ide/src/completion.rs Outdated

+                      // If we have `from .('.' | '...')* <name>` we can suggest `import`.
+                      // We check that we must have at least one `.` because we don't want to suggest `import` for `from <name>`.
+                      //
+                      // If we have `from ('.' | '...')* dotted_name <name>` we can suggest `import`.

Member

BurntSushi Nov 10, 2025

Can you make sure comments are wrapped to 99 columns (inclusive) please? There are some other comments below that need to be wrapped too.

crates/ty_ide/src/completion.rs Outdated

+                  }
+                  None
+              }

Member

BurntSushi Nov 10, 2025

I think this routine should be modeled on this existing function:

ruff/crates/ty_ide/src/completion.rs

Lines 667 to 756 in f44598d

    
           /// Looks for the start of a `from module import <CURSOR>` statement. 
        
           /// 
        
           /// If found, one arbitrary token forming `module` is returned. 
        
           fn import_from_tokens(tokens: &[Token]) -> Option<&Token> { 
        
               use TokenKind as TK; 
        
               /// The number of tokens we're willing to consume backwards from 
        
               /// the cursor's position until we give up looking for a `from 
        
               /// module import <CURSOR>` pattern. The state machine below has 
        
               /// lots of opportunities to bail way earlier than this, but if 
        
               /// there's, e.g., a long list of name tokens for something that 
        
               /// isn't an import, then we could end up doing a lot of wasted 
        
               /// work here. Probably humans aren't often working with single 
        
               /// import statements over 1,000 tokens long. 
        
               /// 
        
               /// The other thing to consider here is that, by the time we get to 
        
               /// this point, ty has already done some work proportional to the 
        
               /// length of `tokens` anyway. The unit of work we do below is very 
        
               /// small. 
        
               const LIMIT: usize = 1_000; 
        
               /// A state used to "parse" the tokens preceding the user's cursor, 
        
               /// in reverse, to detect a "from import" statement. 
        
               enum S { 
        
                   Start, 
        
                   Names, 
        
                   Module, 
        
               } 
        
               let mut state = S::Start; 
        
               let mut module_token: Option<&Token> = None; 
        
               // Move backward through the tokens until we get to 
        
               // the `from` token. 
        
               for token in tokens.iter().rev().take(LIMIT) { 
        
                   state = match (state, token.kind()) { 
        
                       // It's okay to pop off a newline token here initially, 
        
                       // since it may occur when the name being imported is 
        
                       // empty. 
        
                       (S::Start, TK::Newline) => S::Names, 
        
                       // Munch through tokens that can make up an alias. 
        
                       // N.B. We could also consider taking any token here 
        
                       // *except* some limited set of tokens (like `Newline`). 
        
                       // That might work well if it turns out that listing 
        
                       // all possible allowable tokens is too brittle. 
        
                       ( 
        
                           S::Start | S::Names, 
        
                           TK::Name 
        
                           | TK::Comma 
        
                           | TK::As 
        
                           | TK::Case 
        
                           | TK::Match 
        
                           | TK::Type 
        
                           | TK::Star 
        
                           | TK::Lpar 
        
                           | TK::Rpar 
        
                           | TK::NonLogicalNewline 
        
                           // It's not totally clear the conditions under 
        
                           // which this occurs (I haven't read our tokenizer), 
        
                           // but it appears in code like this, where this is 
        
                           // the entire file contents: 
        
                           // 
        
                           //     from sys import ( 
        
                           //         abiflags, 
        
                           //         <CURSOR> 
        
                           // 
        
                           // It seems harmless to just allow this "unknown" 
        
                           // token here to make the above work. 
        
                           | TK::Unknown, 
        
                       ) => S::Names, 
        
                       (S::Start | S::Names, TK::Import) => S::Module, 
        
                       // Munch through tokens that can make up a module. 
        
                       ( 
        
                           S::Module, 
        
                           TK::Name | TK::Dot | TK::Ellipsis | TK::Case | TK::Match | TK::Type | TK::Unknown, 
        
                       ) => { 
        
                           // It's okay if there are multiple module 
        
                           // tokens here. Just taking the last one 
        
                           // (which is the one appearing first in 
        
                           // the source code) is fine. We only need 
        
                           // this to find the corresponding AST node, 
        
                           // so any of the tokens should work fine. 
        
                           module_token = Some(token); 
        
                           S::Module 
        
                       } 
        
                       (S::Module, TK::From) => return module_token, 
        
                       _ => return None, 
        
                   }; 
        
               } 
        
               None 
        
           }

Maybe there is a way to factor out the common logic, but even if there isn't, just copying the approach seems like the right move here. I agree with @MichaReiser above. And notice that the function above is careful to not do too much look-behind on the token sequence. It also handles things like ellipsis, non-logical newlines and so on.

crates/ty_ide/src/completion.rs Outdated

+                  #[test]
+                  fn from_import_two_imports_suggests_import() {
+                      let builder =
+                          completion_test_builder("from collections.abc import Sequence\nfrom typing i<CURSOR>");

Member

BurntSushi Nov 10, 2025

For something really short, I'm fine with the \n here. But this starts to make very long lines. I'd rather see this written out using actual new lines:

        let builder = completion_test_builder(
            "from collections.abc import Sequence
             from typing i<CURSOR>"
        );

The test code should automatically handle dedenting.

Member

BurntSushi commented Nov 10, 2025

@MatthewMckee4 I'm going to take over this PR since we'd like to get this fixed. And I'm planning to work on more stuff around keyword completions this week, which will really want to build on top of this PR. Thank you. :-)

Contributor Author

MatthewMckee4 commented Nov 10, 2025

Cool, apologies for the delays, I've been a little busy. Thank you for the feedback too!

Member

AlexWaygood commented Nov 10, 2025

Cool, apologies for the delays, I've been a little busy.

never feel like you need to apologise for unpaid volunteer work :-) we really appreciate the contribution!!

BurntSushi force-pushed the no-completions-in-from-import branch from 8a98b47 to 446048d Compare

November 10, 2025 16:09

BurntSushi approved these changes

View reviewed changes


          [ty] Don't provide completions when in from <module> i statement

8daeed3

This works principally by copying `from_import_tokens` and tweaking
it a bit to look for `from module <CURSOR>` token sequences. We actually
wind up needing to be a little more careful here to parse `<module>`
correctly to avoid something like `from os.i<CURSOR>` from being treated
as if the `i` is the beginning of the `import` keyword.

Fixes astral-sh/ty#1494

BurntSushi force-pushed the no-completions-in-from-import branch from 446048d to 8daeed3 Compare

November 10, 2025 16:36

BurntSushi merged commit 1d18847 into astral-sh:main

41 checks passed

dcreager added a commit that referenced this pull request


          Merge remote-tracking branch 'origin/main' into dcreager/pep-self

8f66823

* origin/main: (38 commits)
  [ty] Make implicit submodule imports only occur in global scope (#21370)
  [ty] introduce local variables for `from` imports of submodules in `__init__.py(i)` (#21173)
  [`ruff`] Ignore `str()` when not used for simple conversion (`RUF065`) (#21330)
  [ty] implement `typing.NewType` by adding `Type::NewTypeInstance`
  [ty] supress inlay hints for `+1` and `-1` (#21368)
  [ty] Use type context for inference of generic constructors (#20933)
  [ty] Improve generic call expression inference (#21210)
  [ty] supress some trivial expr inlay hints (#21367)
  [`configuration`] Fix unclear error messages for line-length values exceeding `u16::MAX` (#21329)
  [ty] Fix incorrect inference of `enum.auto()` for enums with non-`int` mixins, and imprecise inference of `enum.auto()` for single-member enums (#20541)
  [`refurb`] Detect empty f-strings (`FURB105`) (#21348)
  [ty] provide `import` completion when in `from <name> <name>` statement (#21291)
  [ty] elide redundant inlay hints for function args (#21365)
  Fix syntax error false positive on alternative `match` patterns (#21362)
  Add a new "Opening a PR" section to the contribution guide (#21298)
  [`flake8-simplify`] Fix SIM222 false positive for `tuple(generator) or None` (`SIM222`) (#21187)
  Rebuild ruff binary instead of sharing it across jobs (#21361)
  [ty] Fix `--exclude` and `src.exclude` merging (#21341)
  [ty] Add support for properties that return `Self` (#21335)
  Add upstream linter URL to `ruff linter --output-format=json` (#21316)
  ...

BurntSushi added a commit that referenced this pull request


          [ty] Add from <module> im<CURSOR> completion evaluation task

69f6aa4

Ideally this would have been added as part of #21291, but I forgot.

BurntSushi added a commit that referenced this pull request


          [ty] Add from <module> im<CURSOR> completion evaluation task

ec48a47

Ideally this would have been added as part of #21291, but I forgot.

BurntSushi mentioned this pull request

Add context sensitive keyword completions for in and as in some cases astral-sh/ty#1568

Open

MatthewMckee4 deleted the no-completions-in-from-import branch

December 27, 2025 00:21

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

server ty